Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add validation of filepaths for non-BIDS NWB assets #1173

Merged
merged 2 commits into from
Dec 14, 2022
Merged

Conversation

jwodder
Copy link
Member

@jwodder jwodder commented Dec 13, 2022

Closes #1163.

@jwodder jwodder added minor Increment the minor version when merged NWB cmd-validate labels Dec 13, 2022
@codecov
Copy link

codecov bot commented Dec 13, 2022

Codecov Report

Base: 88.92% // Head: 89.16% // Increases project coverage by +0.23% 🎉

Coverage data is based on head (03854a7) compared to base (a0b0015).
Patch coverage: 98.55% of modified lines in pull request are covered.

❗ Current head 03854a7 differs from pull request most recent head 55df999. Consider uploading reports for the commit 55df999 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1173      +/-   ##
==========================================
+ Coverage   88.92%   89.16%   +0.23%     
==========================================
  Files          76       76              
  Lines        9393     9506     +113     
==========================================
+ Hits         8353     8476     +123     
+ Misses       1040     1030      -10     
Flag Coverage Δ
unittests 89.16% <98.55%> (+0.23%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
dandi/files/_private.py 98.11% <80.00%> (ø)
dandi/cli/tests/test_cmd_validate.py 100.00% <100.00%> (ø)
dandi/cli/tests/test_command.py 100.00% <100.00%> (ø)
dandi/files/__init__.py 93.75% <100.00%> (+15.26%) ⬆️
dandi/files/bases.py 77.30% <100.00%> (+0.57%) ⬆️
dandi/organize.py 83.54% <100.00%> (+0.73%) ⬆️
dandi/tests/fixtures.py 98.60% <100.00%> (+0.03%) ⬆️
dandi/tests/test_files.py 100.00% <100.00%> (ø)
dandi/tests/test_organize.py 95.29% <100.00%> (+0.14%) ⬆️
dandi/validate.py 97.87% <100.00%> (+0.14%) ⬆️
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -29,6 +29,7 @@
from dandi.dandiapi import RemoteAsset, RemoteDandiset, RESTFullAPIClient
from dandi.metadata import get_default_metadata, nwb2asset
from dandi.misctypes import DUMMY_DIGEST, Digest, P
from dandi.organize import validate_organized_path

Check notice

Code scanning / CodeQL

Cyclic import

Import of module [dandi.organize](1) begins an import cycle.
@@ -537,6 +541,29 @@
raise
# TODO: might reraise instead of making it into an error
return _pydantic_errors_to_validation_results([e], str(self.filepath))

from .bids import NWBBIDSAsset

Check notice

Code scanning / CodeQL

Cyclic import

Import of module [dandi.files.bids](1) begins an import cycle.

LABELREGEX = r"[^_*\\/<>:|\"'?%@;.]+"
ORGANIZED_FILENAME_REGEX = (
rf"sub-{LABELREGEX}(_(ses|tis|slice|cell)-{LABELREGEX})*(_[a-z]+(\+[a-z]+)*)?\.nwb"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideally there should be a single data structure describing entities etc we should have in the filename. In principle that potential_fields is the one but I guess can't be used here "as is", unless you enrich that structure with regexes for the values, in particular for extensions etc. Then this regex can be constructed programmatically and we would avoid duplication of this knowledge.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yarikoptic Can tis/slice/cell/probe/obj occur without a preceding ses, or is ses a prerequisite for them?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

according to potential_fields, ses is not mandatory, so other entities could occur without a preceding ses.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: 55df999.

@yarikoptic
Copy link
Member

ok, I dislike that we have duplication of specification ATM between organize and validate but ok -- let's RF it after.

Meanwhlie filed #1174 which found while test driving, and seems to work:

❯ dandi validate XCaMPgf/XCaMPgf_ANM471996_cell01.nwb
[NWBI.check_subject_species_form] XCaMPgf/XCaMPgf_ANM471996_cell01.nwb — Subject species 'mus musculus' should be in latin binomial form, e.g. 'Mus musculus' and 'Homo sapiens'
[NWBI.check_intracellular_electrode_cell_id_exists] XCaMPgf/XCaMPgf_ANM471996_cell01.nwb — Please include a unique cell_id associated with this IntracellularElectrode.
[DANDI.NON_DANDI_FILENAME] XCaMPgf/XCaMPgf_ANM471996_cell01.nwb — Filename does not conform to Dandi standard
[DANDI.NON_DANDI_FOLDERNAME] XCaMPgf/XCaMPgf_ANM471996_cell01.nwb — File is not in folder at root with subject name

although messages are a bit cryptic I guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd-validate minor Increment the minor version when merged NWB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

upload doesn't seem to be checking for valid DANDI (when not BIDS) organization of files/assets
2 participants